exec: add "auto" vectorize setting and make it default #38777

yuzefovich · 2019-07-09T20:34:05Z

First commit renames Vectorize to VectorizeMode and removes
"experimental_" prefix of vectorize setting.

Second commit adds a fourth option "auto" to vectorize execution mode setting
which plans the queries containing only streaming operators via the
vectorized engine. This option is made a default one. Other options
have been renamed: "on" -> "experimental_on" and "always" ->
"experimental_always" to highlight for users the risk of using them.

Fixes: #38620.
Fixes: #38920.

cockroach-teamcity · 2019-07-09T20:34:15Z

This change is

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @solongordon)

pkg/sql/exec_util.go, line 157 at r3 (raw file):

		int64(sessiondata.VectorizeOff):       "off",
		int64(sessiondata.VectorizeOn):        "on",
		int64(sessiondata.VectorizeAlways):    "always",

i think one of the things we had discussed was to rename the "always" value to something like "experimental_on" (and maybe something similar for the current "on" setting?) so that it's more clear to users that it's likely broken. i'm not sure if we reached a firm conclusion, but just wondering if you had considered that.

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @solongordon)

pkg/sql/exec_util.go, line 157 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think one of the things we had discussed was to rename the "always" value to something like "experimental_on" (and maybe something similar for the current "on" setting?) so that it's more clear to users that it's likely broken. i'm not sure if we reached a firm conclusion, but just wondering if you had considered that.

I was actually hoping to reach a consensus about the naming while reviewing this PR, so thanks for starting up the discussion :)

I agree with your point, and I think I'm leaning towards on -> experimental_all and always -> experimental_forced, but I'm definitely open to suggestions so that it's clear that both can be broken and that choosing the latter sounds harsh.

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @solongordon)

pkg/sql/exec_util.go, line 157 at r3 (raw file):

Previously, yuzefovich wrote…

I was actually hoping to reach a consensus about the naming while reviewing this PR, so thanks for starting up the discussion :)

I agree with your point, and I think I'm leaning towards on -> experimental_all and always -> experimental_forced, but I'm definitely open to suggestions so that it's clear that both can be broken and that choosing the latter sounds harsh.

I like that, though it might also be nice to mirror the distsql setting's options:

cockroach/pkg/sql/sessiondata/session_data.go

Lines 206 to 216 in 813b014

    
           const ( 
        
           	// DistSQLOff means that we never distribute queries. 
        
           	DistSQLOff DistSQLExecMode = iota 
        
           	// DistSQLAuto means that we automatically decide on a case-by-case basis if 
        
           	// we distribute queries. 
        
           	DistSQLAuto 
        
           	// DistSQLOn means that we distribute queries that are supported. 
        
           	DistSQLOn 
        
           	// DistSQLAlways means that we only distribute; unsupported queries fail. 
        
           	DistSQLAlways 
        
           )

So we could do off, auto, experimental_on, and experimental_always

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)

pkg/sql/exec_util.go, line 157 at r3 (raw file):

Previously, solongordon (Solon) wrote…

I like that, though it might also be nice to mirror the distsql setting's options:

cockroach/pkg/sql/sessiondata/session_data.go

Lines 206 to 216 in 813b014

const (

// DistSQLOff means that we never distribute queries.

DistSQLOff DistSQLExecMode = iota

// DistSQLAuto means that we automatically decide on a case-by-case basis if

// we distribute queries.

DistSQLAuto

// DistSQLOn means that we distribute queries that are supported.

DistSQLOn

// DistSQLAlways means that we only distribute; unsupported queries fail.

DistSQLAlways

)

So we could do off, auto, experimental_on, and experimental_always

Aside from just using consistent language, I think it would be nice to use auto rather than streaming because later we might want to start gradually enabling non-streaming operators by default. So it could be nice to have a more generic term like "auto" which represents everything we're comfortable enabling by default.

Renames Vectorize to VectorizeMode and removes "experimental_" prefix. Release note: None

yuzefovich · 2019-08-02T17:55:27Z

Alright, finally this is RFAL.

jordanlewis

let's get this baby in!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)

asubiotto · 2019-08-02T19:38:51Z

Should we wait until Monday to enable this so that we don't feel pressured to address any fallout?

jordanlewis · 2019-08-02T19:39:59Z

I think I'd prefer to see this go in so that the nightlies can fail over the weekend and we'll have a fresh crop of stuff to look in by Monday :) Just don't check your work email til next week.

Adds a fourth option "auto" to vectorize execution mode setting which plans the queries containing only streaming operators via the vectorized engine. This option is made a default one. Other options have been renamed: "on" -> "experimental_on" and "always" -> "experimental_always" to highlight for users the risk of using them. Release note: None

yuzefovich · 2019-08-02T22:39:57Z

I ran KV99 tests again:

name             old ops/s   new ops/s   delta
KV99-throughput  21.1k ± 3%  17.5k ± 1%  -17.14%  (p=0.008 n=5+5)

name             old ms/s    new ms/s    delta
KV99-P50          16.7 ± 7%   19.9 ± 0%  +19.16%  (p=0.008 n=5+5)
KV99-Avg          5.12 ± 2%   6.00 ± 0%  +17.19%  (p=0.016 n=5+4)

so a slightly bigger performance hit than we observed previously (probably because of the enhanced SupportsVectorized check).

I agree with Jordan, let's merge it now, and we will see the fresh fallout on Monday.

Thanks for the review and the inputs!

bors r+

38777: exec: add "auto" vectorize setting and make it default r=yuzefovich a=yuzefovich First commit renames Vectorize to VectorizeMode and removes "experimental_" prefix of vectorize setting. Second commit adds a fourth option "auto" to vectorize execution mode setting which plans the queries containing only streaming operators via the vectorized engine. This option is made a default one. Other options have been renamed: "on" -> "experimental_on" and "always" -> "experimental_always" to highlight for users the risk of using them. Fixes: #38620. Fixes: #38920. Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

craig · 2019-08-02T23:28:29Z

Build succeeded

GitHub CI (Cockroach)

yuzefovich requested review from jordanlewis, solongordon, asubiotto and a team July 9, 2019 20:34

rafiss reviewed Jul 9, 2019

View reviewed changes

yuzefovich commented Jul 9, 2019

View reviewed changes

yuzefovich force-pushed the vec-setting branch 2 times, most recently from b0ca931 to cc555b2 Compare July 9, 2019 23:14

yuzefovich added the do-not-merge bors won't merge a PR with this label. label Jul 9, 2019

yuzefovich force-pushed the vec-setting branch from cc555b2 to 8849b1c Compare July 10, 2019 02:02

solongordon reviewed Jul 10, 2019

View reviewed changes

yuzefovich force-pushed the vec-setting branch 13 times, most recently from 9bb4e3a to b174338 Compare July 17, 2019 17:33

yuzefovich force-pushed the vec-setting branch 9 times, most recently from 8db9b23 to effc078 Compare August 2, 2019 15:05

sql: minor modifications around vectorize setting

64e155d

Renames Vectorize to VectorizeMode and removes "experimental_" prefix. Release note: None

yuzefovich force-pushed the vec-setting branch 5 times, most recently from 4631246 to fa5f826 Compare August 2, 2019 17:54

yuzefovich removed the do-not-merge bors won't merge a PR with this label. label Aug 2, 2019

jordanlewis approved these changes Aug 2, 2019

View reviewed changes

yuzefovich force-pushed the vec-setting branch from fa5f826 to 2900184 Compare August 2, 2019 21:36

craig bot merged commit 2900184 into cockroachdb:master Aug 2, 2019

yuzefovich mentioned this pull request Aug 3, 2019

Add sql.defaults.experimental_vectorized setting to experimental features list cockroachdb/docs#4953

Closed

rmloveland mentioned this pull request Aug 5, 2019

Update vectorized setting with new name and defaults cockroachdb/docs#5141

Closed

yuzefovich deleted the vec-setting branch August 30, 2019 19:38

knz mentioned this pull request Nov 10, 2019

User-facing changes in 19.2 that were not picked up in release notes cockroachdb/docs#5819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exec: add "auto" vectorize setting and make it default #38777

exec: add "auto" vectorize setting and make it default #38777

yuzefovich commented Jul 9, 2019 •

edited

Loading

cockroach-teamcity commented Jul 9, 2019

rafiss left a comment

yuzefovich left a comment

solongordon left a comment

solongordon left a comment

yuzefovich commented Aug 2, 2019

jordanlewis left a comment

asubiotto commented Aug 2, 2019

jordanlewis commented Aug 2, 2019

yuzefovich commented Aug 2, 2019

craig bot commented Aug 2, 2019

	const (
	// DistSQLOff means that we never distribute queries.
	DistSQLOff DistSQLExecMode = iota
	// DistSQLAuto means that we automatically decide on a case-by-case basis if
	// we distribute queries.
	DistSQLAuto
	// DistSQLOn means that we distribute queries that are supported.
	DistSQLOn
	// DistSQLAlways means that we only distribute; unsupported queries fail.
	DistSQLAlways
	)

exec: add "auto" vectorize setting and make it default #38777

exec: add "auto" vectorize setting and make it default #38777

Conversation

yuzefovich commented Jul 9, 2019 • edited Loading

cockroach-teamcity commented Jul 9, 2019

rafiss left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

yuzefovich commented Aug 2, 2019

jordanlewis left a comment

Choose a reason for hiding this comment

asubiotto commented Aug 2, 2019

jordanlewis commented Aug 2, 2019

yuzefovich commented Aug 2, 2019

craig bot commented Aug 2, 2019

Build succeeded

yuzefovich commented Jul 9, 2019 •

edited

Loading